Fix: bound explicit EC-domain field lengths to prevent buffer overflow - #11273
Fix: bound explicit EC-domain field lengths to prevent buffer overflow#11273jackctj117 wants to merge 2 commits into
Conversation
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Pull request overview
This PR hardens explicit EC-domain parameter decoding against malformed DER that could trigger buffer overflows when converting explicit parameters (prime, A, B, order, and derived base-point coordinates) into fixed-size hex-string buffers.
Changes:
- Added explicit field-length bounds in
EccSpecifiedECDomainDecode()to reject DER explicit-parameter fields larger thanMAX_ECC_BYTES. - Added regression tests that construct explicit-parameter SubjectPublicKeyInfo blobs with oversized fields and verify rejection with
ASN_PARSE_E. - Integrated the new regression test into both the API test suite and
wolfcrypt/test/test.cECC tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
wolfcrypt/src/asn.c |
Adds MAX_ECC_BYTES bounds for explicit EC-domain fields during template-based ASN decoding. |
tests/api/test_ecc.c |
Adds API-level regression test that builds explicit-parameter SPKI inputs and checks acceptance/rejection behavior. |
tests/api/test_ecc.h |
Declares and registers the new API regression test in the ECC test group. |
wolfcrypt/test/test.c |
Adds a second regression test variant in the wolfCrypt test harness and wires it into ecc_test(). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| { | ||
| byte pub[68]; | ||
| pub[0] = 0x00; | ||
| pub[1] = 0x04; | ||
| for (i = 0; i < 32; i++) pub[2 + i] = 0x44; | ||
| for (i = 0; i < 32; i++) pub[34 + i] = 0x55; | ||
| bo = ecc_ssdd_tlv(body, bo, 0x03, pub, 66); /* pubkey BIT STRING */ | ||
| } |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11273
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 6
6 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| static const byte cof1[1] = { 0x01 }; | ||
| byte num[512]; | ||
| byte inner[1024]; | ||
| byte body[2048]; |
There was a problem hiding this comment.
DER builder puts 3.6 KB of fixed buffers on the stack in wolfcrypt/test/test.c · Buffer safety in test payload builders
num[512], inner[1024] and body[2048] add ~3.6 KB to the stack frame in ecc_test()'s call chain, unguarded by WOLFSSL_SMALL_STACK. test.c is compiled into MCU targets as a smoke test and heap-allocates buffers as small as 1280 bytes there; this frame overflows such stacks. The actual maximum needed is a few hundred bytes.
Fix: Right-size the buffers to the required maximum or allocate them from HEAP_HINT under WOLFSSL_SMALL_STACK, as elsewhere in test.c.
|
retest this please |
This pull request addresses a security issue (issue 11288) related to potential buffer overflows when decoding explicit EC domain parameters from DER-encoded data. The main change is to ensure that all explicit-parameter field lengths (prime, coordinate, A, B, and order) are properly bounded to prevent overflows. It also adds regression tests to verify the fix and prevent future regressions.
Security and correctness improvements:
EccSpecifiedECDomainDecodeto reject DER-encoded EC domain parameters with field lengths exceedingMAX_ECC_BYTES, preventing buffer overflows and ensuring safe handling of explicit EC-domain parameters.Testing and regression coverage:
test_wc_EccPublicKeyDecode_specifiedOverflowintest_ecc.cthat builds DER-encoded EC public keys with oversized fields to verify that the new length checks correctly reject them, and that boundary cases are accepted.test_ecc.hand included it in the ECC test group for automated test runs. [1] [2]ecc_ssdd_overflow_test) inwolfcrypt/test/test.cfor additional coverage, including helper functions for DER construction, and integrated it into the main ECC test routine. [1] [2]These changes collectively harden the ECC decoding logic against malformed or malicious input and ensure that future changes will be tested for this class of vulnerability.